Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/#4716 管理者ページ-受注一覧における検索条件の修正 #4766

Merged
merged 6 commits into from
Nov 17, 2020
Merged

Feature/#4716 管理者ページ-受注一覧における検索条件の修正 #4766

merged 6 commits into from
Nov 17, 2020

Conversation

morry48
Copy link
Contributor

@morry48 morry48 commented Nov 7, 2020 •
edited by okazy
Loading

概要(Overview・Refs Issue)

#4716

方針(Policy)

注文者名（カナ）については記載がありませんでしたが、利便性を考えると「カナ」の場合もフルネームで検索できる方がいいと思いますので、こちらも対応しております。

実装に関する補足(Appendix)

特にありません。

テスト（Test)

#4716 (comment)
・上記不可の項目が検索可能となること
・カナ入力においてもフルネームで検索可能であること
・既存機能の注文番号・お名前・会社名・メールアドレス・電話番号で検索可能なこと（デグレを起こしていないこと）

相談（Discussion）

受注一覧-詳細検索-注文者名（カナ）においてスペースを入れて検索しようとすると、
ec-cube/src/Eccube/Form/Type/Admin/SearchOrderTypeでスペースを含めていないため、不正入力扱いになります。
仕様的にOKでしょうか。（スペースも入力可能とする必要はないでしょうか）
ちなみに（注文番号・お名前・会社名・メールアドレス・電話番号）の方で検索すると「ヤマダ　タロウ」のようにスペースを含めたカナ検索は可能となっています。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更
  • フックポイントの呼び出しタイミングの変更
  • フックポイントのパラメータの削除・データ型の変更
  • twigファイルに渡しているパラメータの削除・データ型の変更
  • Serviceクラスの公開関数の、引数の削除・データ型の変更
  • 入出力ファイル(CSVなど)のフォーマット変更

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@morry48 morry48 changed the title Feature/#4716 fix adminpage order search Feature/#4716 管理者ページ-受注一覧における検索条件の修正 Nov 8, 2020
@morry48
Copy link
Contributor Author

morry48 commented Nov 8, 2020

unitテストが落ちており確認しましたところ
Eccube\Tests\Web\Admin\Order\OrderControllerTest::testSearchOrderByName
で検索結果に差異が出ております。

テストの記述に問題があるのではないかと思いましてご相談です。

端的に言うと
$this->expected は「完全一致」
$this->actualは「like検索」
となっているため、
仮にテスト対象が「太郎商事」とした場合で、
「山田太郎商事」という会社名がdtb_orderのレコードに存在する際に、テスト結果に差異が出てしまうのではないかという点です。

①実行しているテストの内容としては必ず１個以上データベースに存在する$companyNameを定義し、
dtb_orderテーブルのcompany_nameカラムと一致(完全一致)しているものを検索し、$OrderListのレコード数を出す。
②クライアントから、「multi(注文番号・お名前・会社名・メールアドレス・電話番号)」に①で定義した$companyNameが入力された状態で検索を実行しその結果を出す。

今回のテストのDetailsはexpectedの方が数が多くなっているため、自動テストが落ちている原因は別かもしれないのですが、ご確認いただけますと幸甚でございます。

@okazy okazy added the improvement 機能改善 label Nov 9, 2020
@okazy okazy added this to the 4.0.x milestone Nov 9, 2020
okazy
okazy reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@okazy okazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morry48
プルリクありがとうございます。
変更内容みて一点コメントしました。
テストが落ちている原因はまだ見れていません。

src/Eccube/Repository/OrderRepository.php Outdated
'o.order_no LIKE :likemulti OR o.email LIKE :likemulti OR o.phone_number LIKE :likemulti')
->setParameter('multi', $multi)
->setParameter('likemulti', '%'.$searchData['multi'].'%');
->setParameter('order_id', $id)
Copy link
Contributor

@okazy okazy Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

より正確でわかりやすい key に変えていただいてわかりやすくなったと思いますが、カスタマイズされて使われている可能性もありますので、マイナーバージョンではkeyは変更しない方がいい以下と思いました。
order_id -> multi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど！
注文者の実装を参考にしたのですが、プラグインへの影響まで考慮できていませんでした。

修正のコミット追加しておりますので、ご確認のほど、よろしくおねがいします。

@morry48 morry48 force-pushed the feature/#4716_fix_adminpage_order_search branch from e6e8afd to 42609b5 Compare November 11, 2020 15:25
@okazy okazy mentioned this pull request Nov 12, 2020
Merged
@okazy
Copy link
Contributor

okazy commented Nov 12, 2020 •
edited
Loading

@morry48
ご対応ありがとうございます！

テストが落ちていた原因がわかりました。
会社名はスペースの入力ができます。
例えば「株式会社　ほげほげ」という会社名だった場合に、前処理でスペースが除去され、「株式会社ほげほげ」で検索されてしまっていたため、検索結果がヒットしなくなっていました。

受注のマルチ検索で、会社名はスペースを除去せず検索できるよう修正してプルリクを出しましたのでご確認とマージをお願いできますでしょうか。

またOrderRepositoryの名前の検索に関するテストがなかったので新規追加しておきました。

morry48#1

@morry48
Merge pull request #1 from okazy/feature/#4716_fix_adminpage_order_se…
c7c415d 
…arch

受注のマルチ検索で、会社名はスペースを除去せず検索
@morry48
Copy link
Contributor Author

morry48 commented Nov 14, 2020

会社名はスペース有りなんですね。仕様を考慮できていませんでした。

アプルーブしてマージにましたので、ご確認のほど宜しくおねがいします。

okazy
okazy approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@okazy okazy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます！

会社名のスペースありは私も把握していない仕様でした。
勉強になりました。

修正内容、テスト、動作共に問題ありません。

@okazy okazy modified the milestones: 4.0.x, 4.0.6 Nov 16, 2020
@okazy okazy merged commit 7738e30 into EC-CUBE:4.0 Nov 17, 2020
@okazy
Copy link
Contributor

okazy commented Nov 17, 2020

@morry48
ありがとうございます！取り込みました 🎉

This was referenced Nov 17, 2020
Closed
Open
@chihiro-adachi chihiro-adachi modified the milestones: 4.0.6, 4.1 Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okazy okazy okazy approved these changes

Assignees
No one assigned
Labels
improvement 機能改善 Status: ready-for-merge
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@morry48 @okazy @chihiro-adachi